fix: Use correct parsing for stackframes #6908
Open
+269
−368
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahsmartin
force-pushed
the
mxRewriting
branch
from
73c4c82 to
3985141
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6908 +/- ##
=============================================
- Coverage 85.076% 85.037% -0.039%
=============================================
Files 453 455 +2
Lines 27674 27690 +16
Branches 12166 12160 -6
=============================================
+ Hits 23544 23547 +3
- Misses 4087 4099 +12
- Partials 43 44 +1
... and 42 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
noahsmartin
force-pushed
the
mxRewriting
branch
from
3985141 to
ced502e
Compare
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 762a701 | 1225.95 ms | 1244.49 ms | 18.54 ms |
| 61414e8 | 1225.49 ms | 1254.28 ms | 28.79 ms |
| 588dd7c | 1235.11 ms | 1241.76 ms | 6.65 ms |
| f5666e7 | 1227.08 ms | 1260.18 ms | 33.10 ms |
| b9aacb6 | 1230.42 ms | 1251.00 ms | 20.58 ms |
| 16f6edc | 1234.02 ms | 1269.67 ms | 35.65 ms |
| b709887 | 1193.52 ms | 1216.74 ms | 23.22 ms |
| 5fc3364 | 1222.36 ms | 1252.33 ms | 29.96 ms |
| 1fe932f | 1231.92 ms | 1253.44 ms | 21.52 ms |
| 80538ca | 1216.70 ms | 1253.92 ms | 37.22 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 762a701 | 23.75 KiB | 1022.78 KiB | 999.03 KiB |
| 61414e8 | 23.75 KiB | 867.69 KiB | 843.94 KiB |
| 588dd7c | 23.75 KiB | 938.33 KiB | 914.58 KiB |
| f5666e7 | 23.75 KiB | 963.18 KiB | 939.43 KiB |
| b9aacb6 | 23.75 KiB | 913.64 KiB | 889.89 KiB |
| 16f6edc | 23.74 KiB | 1022.94 KiB | 999.19 KiB |
| b709887 | 23.75 KiB | 1.01 MiB | 1016.14 KiB |
| 5fc3364 | 23.75 KiB | 1.00 MiB | 1006.00 KiB |
| 1fe932f | 23.75 KiB | 913.63 KiB | 889.88 KiB |
| 80538ca | 23.75 KiB | 989.99 KiB | 966.24 KiB |
Previous results on branch: mxRewriting
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 44da6c5 | 1234.22 ms | 1266.02 ms | 31.80 ms |
| 8824c94 | 1226.98 ms | 1262.65 ms | 35.67 ms |
| c5b3f7a | 1220.18 ms | 1259.44 ms | 39.25 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 44da6c5 | 24.14 KiB | 1.03 MiB | 1.00 MiB |
| 8824c94 | 24.14 KiB | 1.01 MiB | 1014.88 KiB |
| c5b3f7a | 24.14 KiB | 1.01 MiB | 1013.45 KiB |
noahsmartin
force-pushed
the
mxRewriting
branch
2 times, most recently
from
d978079 to
169cf94
Compare
noahsmartin
force-pushed
the
mxRewriting
branch
2 times, most recently
from
432f4ad to
a7949ab
Compare
noahsmartin
force-pushed
the
mxRewriting
branch
from
a7949ab to
0c4f788
Compare
noahsmartin
requested review from
itaybre,
philipphofmann and
philprime
as code owners
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| [self captureMXEvent:callStackTree | ||
| params:params | ||
| diagnosticJSON:[diagnostic JSONRepresentation]]; | ||
| [self captureMXEvent:callStackTree params:params diagnosticJSON:[NSData data]]; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
noahsmartin
force-pushed
the
mxRewriting
branch
from
0c4f788 to
db09d78
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // that just contains the most common callstack. | ||
| func sentryMXBacktrace(inAppLogic: SentryInAppLogic?, handled: Bool) -> [SentryThread] { | ||
| callStacks.map { callStack in | ||
| let thread = SentryThread(threadId: 0) |
There was a problem hiding this comment.
Bug: All threads incorrectly assigned thread ID 0
In sentryMXBacktrace(), all threads are assigned threadId: 0 regardless of their position in the callStacks array. The old Objective-C code used an incrementing counter to assign unique thread IDs (@(i)). When callStackPerThread is true (e.g., for crash diagnostics), multiple call stacks represent different threads and need distinct IDs. Using callStacks.enumerated().map would provide the index needed for unique thread IDs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4040
The problem was that for some events such as app hangs and CPUDiagnostics the metric kit payload contains an entire flamegraph. This change parses the flame graph into samples, and then picks the longest duration sample as the one we use for a stack trace. It also fixes over-counting the CPUDiagnostics which was emitting more than one event per exception. This is also fixed by identifying the longest duration sample.
Eventually we should support showing the full flame graph on the frontend, but with this PR the bug is fixed and we can consider that a future improvement.
I tested this on test flight by triggering crashes and verifying they are still correct